New FontData Constructor to create a deep copy #2128

ShahzaibIbrahim · 2025-05-08T11:12:44Z

This change will give us a chance to use proper channel to create a deep copy of FontData object instead of using FontData(fd.toString)

github-actions · 2025-05-08T11:24:43Z

Test Results

539 files - 6 539 suites - 6 39m 48s ⏱️ + 4m 14s
4 341 tests - 36 4 325 ✅ - 34 15 💤 - 3 1 ❌ +1
16 614 runs - 33 16 475 ✅ - 31 138 💤 - 3 1 ❌ +1

For more details on these failures, see this check.

Results for commit 46cfcee. ± Comparison against base commit 63048a2.

This pull request removes 37 and adds 1 tests. Note that renamed tests count towards both.

AllWin32Tests org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageDataForDifferentFractionalZoomsShouldBeDifferent
AllWin32Tests org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageShouldHaveDimesionAsPerZoomLevel
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testByteArrayTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testFileTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testHtmlTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromCopiedImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageData
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageDataFromImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testRtfTransfer
…

org.eclipse.swt.tests.junit.Test_org_eclipse_swt_graphics_FontData ‑ test_ConstructorLjava_lang_FontData

♻️ This comment has been updated with latest results.

This change will give us a chance to use proper channel to create a deep copy of FontData object instead of using FontData(fd.toString)

Using the new constructor FontData(fontData) to deep copy the object.

HeikoKlare

Having a proper copy constructor for FontData sounds reasonable to me.
However, the implementations are no proper copy constructors. They seem to be copies of the String-based constructors which is only able to copy the data present in the string, while a proper copy constructor can simply deep-copy all the data from the existing FontData object.

HeikoKlare · 2025-05-30T10:27:30Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/FontData.java

+	if (fontData.data != null) {
+		LOGFONT newData = new LOGFONT();
+		setHeight(fontData.getHeightF());
+		newData.lfHeight = fontData.data.lfHeight;
+		newData.lfWidth = fontData.data.lfWidth;
+		newData.lfEscapement = fontData.data.lfEscapement;
+		newData.lfOrientation = fontData.data.lfOrientation;
+		newData.lfWeight = fontData.data.lfWeight;
+		newData.lfItalic = fontData.data.lfItalic;
+		newData.lfUnderline = fontData.data.lfUnderline;
+		newData.lfStrikeOut = fontData.data.lfStrikeOut;
+		newData.lfCharSet = fontData.data.lfCharSet;
+		newData.lfOutPrecision = fontData.data.lfOutPrecision;
+		newData.lfClipPrecision = fontData.data.lfClipPrecision;
+		newData.lfQuality = fontData.data.lfQuality;
+		newData.lfPitchAndFamily = fontData.data.lfPitchAndFamily;
+
+		// Deep copy of the char array lfFaceName
+		if (fontData.data.lfFaceName != null) {
+			newData.lfFaceName = new char[fontData.data.lfFaceName.length];
+			System.arraycopy(fontData.data.lfFaceName, 0, newData.lfFaceName, 0, fontData.data.lfFaceName.length);
+		}
+
+		this.data = newData;
+	} else {
+		this.data = null;
+	}


This could be placed in a dedicated private static cloneLogfont method, in which you can also early return on that font being null.

HeikoKlare · 2025-05-30T10:30:06Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/FontData.java

+
+	if (fontData.data != null) {
+		LOGFONT newData = new LOGFONT();
+		setHeight(fontData.getHeightF());


Instead of setting specific fields via setters, shouldn't we copy over all data in a proper copy constructor, i.e., also the height, lang, country and variant fields? This seems to be a copy-over from the String-based constructor, in which some original data is missing and needs to be re-initialized from the data being given using according setters.

HeikoKlare · 2025-05-30T10:32:17Z

bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/FontData.java

+ * </ul>
+ * @since 3.130
+ */
+public FontData(FontData fontData) {


Same as for the other implementations: why not simply copy the fields from the original font data instead of using setters with additional checks that may not be useful when already having a properly initialized font data hand? In addition, some data like lang, country, and variant seem to not be copied by the copy constructors.

HeikoKlare · 2025-05-30T10:33:24Z

bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/FontData.java

@@ -186,6 +186,26 @@ public FontData(String string) {
 	}
 }

+/**
+ * Constructs a new FontData copy


Suggested change

* Constructs a new FontData copy

* Constructs a deep copy of the given font data object.

HeikoKlare · 2025-05-30T10:33:39Z

bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/FontData.java

+/**
+ * Constructs a new FontData copy
+ *
+ * @param fontData the FondData object for which copy is needed to be made


Suggested change

* @param fontData the FondData object for which copy is needed to be made

* @param fontData the FontData object to copy

ShahzaibIbrahim force-pushed the master-296 branch from 0441bce to d32fe1c Compare May 8, 2025 11:55

ShahzaibIbrahim marked this pull request as ready for review May 8, 2025 11:56

ShahzaibIbrahim linked an issue May 8, 2025 that may be closed by this pull request

Improve FontData Copying: Replace String Constructor with Safer Approach vi-eclipse/Eclipse-Platform#296

Open

ShahzaibIbrahim force-pushed the master-296 branch from d32fe1c to c7a5291 Compare May 9, 2025 14:02

ShahzaibIbrahim added 3 commits May 13, 2025 12:16

[win32] Introducing new FontData Constructor to create a deep copy

50575a9

This change will give us a chance to use proper channel to create a deep copy of FontData object instead of using FontData(fd.toString)

Replacing occurrences of FontData(String) to Deep copy the object

50258e6

Using the new constructor FontData(fontData) to deep copy the object.

Addition of Deep Copy Methods for FontData in Cocoa and GTK

46cfcee

ShahzaibIbrahim force-pushed the master-296 branch from c7a5291 to 46cfcee Compare May 13, 2025 10:16

HeikoKlare requested changes May 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

New FontData Constructor to create a deep copy #2128

New FontData Constructor to create a deep copy #2128

Uh oh!

ShahzaibIbrahim commented May 8, 2025

Uh oh!

github-actions bot commented May 8, 2025 •

edited

Loading

Uh oh!

HeikoKlare left a comment

Uh oh!

HeikoKlare May 30, 2025

Uh oh!

HeikoKlare May 30, 2025

Uh oh!

HeikoKlare May 30, 2025

Uh oh!

HeikoKlare May 30, 2025

Uh oh!

HeikoKlare May 30, 2025

Uh oh!

Uh oh!

	* Constructs a new FontData copy
	* Constructs a deep copy of the given font data object.

	* @param fontData the FondData object for which copy is needed to be made
	* @param fontData the FontData object to copy

New FontData Constructor to create a deep copy #2128

Are you sure you want to change the base?

New FontData Constructor to create a deep copy #2128

Uh oh!

Conversation

ShahzaibIbrahim commented May 8, 2025

Uh oh!

github-actions bot commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

HeikoKlare left a comment

Choose a reason for hiding this comment

Uh oh!

HeikoKlare May 30, 2025

Choose a reason for hiding this comment

Uh oh!

HeikoKlare May 30, 2025

Choose a reason for hiding this comment

Uh oh!

HeikoKlare May 30, 2025

Choose a reason for hiding this comment

Uh oh!

HeikoKlare May 30, 2025

Choose a reason for hiding this comment

Uh oh!

HeikoKlare May 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented May 8, 2025 •

edited

Loading